👋 Welcome back jbhateja! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@jatin-bhateja The following labels will be automatically applied to this pull request:

• core-libs
• hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

/label add hotspot-compiler-dev

@jatin-bhateja
The hotspot-compiler label was successfully added.

Webrevs

• 00: Full (c60d533c)

Some quick comments.

We should be consistent in the naming, and rename Halfloat* to Float16*.

I concur, especially since there are multiple 16-bit floating-point formats in use including the IEEE 754 float16 as well as bfloat16.

We already have a lot of things in the codebase now from previous issues that use HF everywhere, for example some node names, and the type. Should we maybe rename all of them to F16, or something else? Open question, not sure of the answer yet.

We already have a lot of things in the codebase now from previous issues that use HF everywhere, for example some node names, and the type. Should we maybe rename all of them to F16, or something else? Open question, not sure of the answer yet.

I was only referring to the Java code, esp. the new public classes so they align with the Float16 element type. I do think it worthwhile to align so we are consistent across the platform. Revisiting the names in HotSpot, and their internal connection in Java, could be done in a separate PR?

Revisiting the names in HotSpot, and their internal connection in Java, could be done in a separate PR?

Yes, exactly. Maybe even in a quick renaming PR before this issue. Would be quickly reviewed, and would allow us to see complete consistency going forward with this PR here.

Hi @PaulSandoz , Thanks for your comments. Please find below my responses.

When you generate the fallback code for unary/binary etc can you push the carrier type and conversations into the uOp/bOp implementations so you don't have to explicitly operate on the carrier type and do the conversions as you do now e.g.,:

v0.uOp(m, (i, a) -> float16ToShortBits(Float16.valueOf(-(shortBitsToFloat16(($type$)a).floatValue()))));

Currently, uOp and uOpTemplates are part of the scaffolding logic and are sacrosanct; they are shared by various abstracted vector classes, and their semantics are defined by the lambda expression. I agree that explicit conversion in lambdas looks verbose, but moving them to uOpTemplate may fracture the lambda expression such that part of its semantics, i.e,. conversions, will seep into uOpTemplate, while what will appear at the surface will be the expression operating over primitive float values; this may become very confusing.

The transition of intrinsic arguments from vsp.elementType() to vsp.carrierType(), vsp.operType() is a little unfortunate. Is this because HotSpot cannot directly refer to the Float16 class from the incubating module?

Yes, the idea here was to clearly differentiate b/w elemType and carrierType and avoid passing Float16.class as an argument to intrinsic entry points. Unlike the VectorSupport class, Float16 is part of the incubating module and cannot be directly exposed to VM, i.e., we cannot create a vmSymbol for it during initialization. This would have made all the lane type checks within inline expand name-based rather than efficient symbol lookup.

Requiring two arguments means they can get out of sync. Previously the class provided all the information needed, now
arguably the type does.

Yes, from the compiler standpoint point all we care about is the carrier type, which determines the vector lane size. This is augmented with operation kind (PRIM / FP16) to differentiate a short vector lane from a float16 vector lane. Apart from this, we need to pass the VectorBox type to wrap the vector IR.

Some quick comments.
We should be consistent in the naming, and rename Halfloat* to Float16*.

I concur, especially since there are multiple 16-bit floating-point formats in use including the IEEE 754 float16 as well as bfloat16.

There are nomenclature issues that I am facing. Currently, all the Float16 concrete classes use the Halffloat prefix i.e., Halffloat64Vector, Halffloat128Vector; converting these to Float16 looks a little confusing, i.e., Float1664Vector, Float16128Vector, etc Kindly suggest a better name to represent these classes.